You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
well, this kinda sucks but i think it's better - #11622
Okay, so basically, all of this code is bad, and I should feel bad about
it, but this PR makes some of it a bit better at the cost of changing a
lot of code all at once.
record scratch You're probably wondering how I got here. Basically, it
turns out that the undo actions for the sis_ensure_registered and sim_ensure_destination_propolis nodes in the instance-start and
instance-migrate sagas, respectively, are both a bit wrong. Both of
these nodes' forward actions are responsible for sending a vmm_ensure
request to a sled-agent, to start the Propolis zone to start an instance
or for the migration destination of an instance, respectively. When
these sagas unwind, they must ensure that the corresponding VMM is unregistered --- i.e. that the sled-agent is told to forcefully
terminate that Propolis zone --- since the control plane does not
believe that an instance is running there. If we fail to ensure that the
zone is unregistered when these sagas unwind, we run the risk of
"leaking" a Propolis zone which the control plane would believe does not
exist but which may believe itself to be running the instance (if the
start saga got far enough along before unwinding).
Presently, the unwinding actions for these nodes are wrong in several
ways. The first such way (which is not addressed in this PR) is that
they really should run regardless of whether or not the forward action
succeded: if the forward action fails and we start unwinding there, we may still have sent an vmm_ensure request to the sled-agent, and the
VMM may still exist. That's part of what leads to the bug described in #11560, and I had started on trying to fix this in #11608 when I
stumbled over all the other stuff that's wrong here.
The other things that are wrong are:
When an error is returned by the sled-agent that affirmatively
indicates the VMM does NOT exist on that sled (i.e. a 404 with the
error code NO_SUCH_INSTANCE), the unwinding action will...mark the
VMM record as Failed, and then return an error and get the saga
stuck. This is a bit weird of us to do here. While we generally
do handle these 404 NO_SUCH_INSTANCE errors by failing the VMM, we
probably don't want to here: that error means the Propolis zone definitely doesn't exist, which is precisely what we were trying
to ensure in the first place. And, indeed, the else branch, which
would execute on any error except for those which positively
indicates the Propolis zone no longer exists, logs "instance already
unregistered from sled", which is actually what the case where we do get the NO_SUCH_INSTANCE error represents. So this is
basically doing the exat opposite of what it's clearly intended to
do.1
If we encounter a transient network error here, we incorrectly assume
that it means the instance has been unregistered, as described in
(1), and therefore do not retry. This means we will continue
unwinding, leaving behind the VMM which we had meant to unregister,
if we just so happen to hit a network blip while talking to the
sled-agent.
On the other hand, if we encounter a (potentially equally transient)
network error while talking to the database, either while looking
up the VMM record in order to stop it (which is what lands us here), or while we are attempting to mark the VMM record as
failed (which, as I mentioned above, we shouldn't even be doing),
we will return an action_failed error and get the saga permanently
stuck. Which we shouldn't do.
Finally, none of this code handles the fact that the sled may have
been expunged while we were trying to do all of that, which should
probably be treated the same as the VMM no longer existing.
Whew, okay. That was a lot. Instead of all of that nonsense, what the
undo actions really should be doing is:
Try to tell the sled-agent to unregister the VMM.
If that succeeds, great. We can return immediately Ok(()) and
continue unwinding.
If the sled-agent returns a 404 NO_SUCH_INSTANCE error, that means
that the VMM has definitely been destroyed, either because of a
previous idempotent execution of the undo action, because we never
actually ensured it in the first place, or because the whole sled has
restarted. In that case, we can return Ok(()) immediately and
continue unwinding. We do not need to mark the VMM as Failed: in
that case: it will be marked as SagaUnwound by a later unwinding
action.
If we cannot communicate with the sled-agent, we should check to see
if the sled has been expunged. If the sled is expunged, we can safely
assume that the VMM is gone, and return Ok(()) immediately and
continue unwinding (you know the drill). If the sled has not been
expunged, we must retry forever.
If the sled record in the database has been deleted wholesale, well,
that's weird and probably shouldn't have happened, but we can probably also assume that means the VMM is gone, and return Ok(())
immediately and continue unwinding (and only feel a little bit bad
about having done so).
Any other error should be retried forever until the VMM is confirmed
gone by one of the above means. We should never return an error here,
since that gets the whole saga stuck and requires manual intervention,
which has almost never been the correct thing for any saga to do under
any circumstances.
Okay. So having determined that, I set out to try to make it do that.
While trying to do that , I realized that none of the other code
that will send a request to the sled-agent to operate on the instance
ever checks if the sled is expunged, either.2 Upon discovering this,
I figured that we should probably make all the code that tries to
operate on a VMM handle the possibility of the sled having been expunged
similarly to the "VMM definitely gone" 404 error case, which is what
ended up turning this into such a large change.
This branch, therefore, makes the following changes:
Turns SledAgentInstanceError, which is currently just a little
wrapper around a progenitor_client::Error from the
sled-agent-client that we use to special-case the 404 NO_SUCH_INSTANCE error, into an enum of either VmmGone, SledNoLongerInService, or "other".
Added a (somewhat awkwardly-shaped) SledAgentInstanceError::new.
This checks if the error is an error response from the sled-agent
indicating the VMM is gone (as we did previously). In addition, if
the error is a communication error, it also checks if the sled has
been expunged by consulting the database. Eventually, I would also
kinda like to hang the "check if the sled is still in A0" code from [nexus] fail instances if the sled they're on isn't...on #10334 off of this type instead of making that something that only
the instance_watcher background task knows how to do, but I'd
rather do that in a follow-up change.
This is kinda why we don't just use the progenitor-extras GoneCheck stuff here, because we also want to keep the current
handling of the NO_SUCH_INSTANCE error and the associated logic for
constructing VmmFailureReasons from it.
Updated all the call sites that use SledAgentInstanceError to use
the new thing, which is why this diff is so huge. Sorry.
Finally, once all that was done, added a new undo_instance_ensure_registered helper in sagas::instance_common
for actually doing the ensure-unregistered undo action. This has a a
retry loop that actually does the correct thing (as I described
above) instead of the kind of fractally-wrong thing that the instance-start saga's version of this used to do. Now, both sagas
just call this, and it does the right thing.
Return Ok(()) immediately and continue unwinding.
Footnotes
Incidentally, this is one of the only pieces of this whole mess
that is not entirely my fault: although the code in question was
last touched by yours truly, there is a comment above it
explaining the rationale for the incorrect behavior. Although
this comment is also both incorrect in describing the code's
behavior and what it asserts that the code does would be
incorrect if we did do that, it is exceptionally well written in
a way where long-time fans of the show will immediately realize
we have just stumbled across a @gjcolumbo cameo appearance (hi
Greg!). ↩
With the exception of the instance-watcher background task...but
it only checks if the sled has been expunged before trying to
talk to the sled-agent, and doesn't check again upon encountering
a communication error. Sigh. ↩
Okay, so the missing piece here — and the reason we've not actually seen unwinding start sagas get stuck on the unregister action in practice — is that vmm_unregister always returns 200 OK for unknown VMM IDs, making it idemoptent. So, even though we were handling the NO_SUCH_INSTANCE branch in a completely incorrect way, that endpoint would never return it. Thus, I think we should really just change that bit by not wrapping that function's error in a SledAgentInstanceError, and use the progenitor-extra GoneCheck stuff there. I think the rest of the error refactoring is still worth doing independently, but we shouldn't actually see any of it in the unregistered action.
Thus, I think we should really just change that bit by not wrapping that function's error in a SledAgentInstanceError, and use the progenitor-extra GoneCheck stuff there. I think the rest of the error refactoring is still worth doing independently, but we shouldn't actually see any of it in the unregistered action.
#11629 does this part. Proper commit message there to follow. Then, I'll figure out how to nicely base this whole stack of changes atop that.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Okay, so basically, all of this code is bad, and I should feel bad about
it, but this PR makes some of it a bit better at the cost of changing a
lot of code all at once.
record scratch You're probably wondering how I got here. Basically, it
turns out that the undo actions for the
sis_ensure_registeredandsim_ensure_destination_propolisnodes in the instance-start andinstance-migrate sagas, respectively, are both a bit wrong. Both of
these nodes' forward actions are responsible for sending a
vmm_ensurerequest to a sled-agent, to start the Propolis zone to start an instance
or for the migration destination of an instance, respectively. When
these sagas unwind, they must ensure that the corresponding VMM is
unregistered --- i.e. that the sled-agent is told to forcefully
terminate that Propolis zone --- since the control plane does not
believe that an instance is running there. If we fail to ensure that the
zone is unregistered when these sagas unwind, we run the risk of
"leaking" a Propolis zone which the control plane would believe does not
exist but which may believe itself to be running the instance (if the
start saga got far enough along before unwinding).
Presently, the unwinding actions for these nodes are wrong in several
ways. The first such way (which is not addressed in this PR) is that
they really should run regardless of whether or not the forward action
succeded: if the forward action fails and we start unwinding there, we
may still have sent an
vmm_ensurerequest to the sled-agent, and theVMM may still exist. That's part of what leads to the bug described in
#11560, and I had started on trying to fix this in #11608 when I
stumbled over all the other stuff that's wrong here.
The other things that are wrong are:
When an error is returned by the sled-agent that affirmatively
indicates the VMM does NOT exist on that sled (i.e. a 404 with the
error code
NO_SUCH_INSTANCE), the unwinding action will...mark theVMM record as
Failed, and then return an error and get the sagastuck. This is a bit weird of us to do here. While we generally
do handle these 404 NO_SUCH_INSTANCE errors by failing the VMM, we
probably don't want to here: that error means the Propolis zone
definitely doesn't exist, which is precisely what we were trying
to ensure in the first place. And, indeed, the
elsebranch, whichwould execute on any error except for those which positively
indicates the Propolis zone no longer exists, logs "instance already
unregistered from sled", which is actually what the case where we
do get the
NO_SUCH_INSTANCEerror represents. So this isbasically doing the exat opposite of what it's clearly intended to
do.1
If we encounter a transient network error here, we incorrectly assume
that it means the instance has been unregistered, as described in
(1), and therefore do not retry. This means we will continue
unwinding, leaving behind the VMM which we had meant to unregister,
if we just so happen to hit a network blip while talking to the
sled-agent.
On the other hand, if we encounter a (potentially equally transient)
network error while talking to the database, either while looking
up the VMM record in order to stop it (which is what lands us
here), or while we are attempting to mark the VMM record as
failed (which, as I mentioned above, we shouldn't even be doing),
we will return an
action_failederror and get the saga permanentlystuck. Which we shouldn't do.
Finally, none of this code handles the fact that the sled may have
been expunged while we were trying to do all of that, which should
probably be treated the same as the VMM no longer existing.
Whew, okay. That was a lot. Instead of all of that nonsense, what the
undo actions really should be doing is:
Ok(())andcontinue unwinding.
NO_SUCH_INSTANCEerror, that meansthat the VMM has definitely been destroyed, either because of a
previous idempotent execution of the undo action, because we never
actually ensured it in the first place, or because the whole sled has
restarted. In that case, we can return
Ok(())immediately andcontinue unwinding. We do not need to mark the VMM as
Failed: inthat case: it will be marked as
SagaUnwoundby a later unwindingaction.
if the sled has been expunged. If the sled is expunged, we can safely
assume that the VMM is gone, and return
Ok(())immediately andcontinue unwinding (you know the drill). If the sled has not been
expunged, we must retry forever.
sledrecord in the database has been deleted wholesale, well,that's weird and probably shouldn't have happened, but we can
probably also assume that means the VMM is gone, and return
Ok(())immediately and continue unwinding (and only feel a little bit bad
about having done so).
gone by one of the above means. We should never return an error here,
since that gets the whole saga stuck and requires manual intervention,
which has almost never been the correct thing for any saga to do under
any circumstances.
Okay. So having determined that, I set out to try to make it do that.
While trying to do that , I realized that none of the other code
that will send a request to the sled-agent to operate on the instance
ever checks if the sled is expunged, either.2 Upon discovering this,
I figured that we should probably make all the code that tries to
operate on a VMM handle the possibility of the sled having been expunged
similarly to the "VMM definitely gone" 404 error case, which is what
ended up turning this into such a large change.
This branch, therefore, makes the following changes:
Turns
SledAgentInstanceError, which is currently just a littlewrapper around a
progenitor_client::Errorfrom thesled-agent-client that we use to special-case the 404
NO_SUCH_INSTANCEerror, into an enum of eitherVmmGone,SledNoLongerInService, or "other".Added a (somewhat awkwardly-shaped)
SledAgentInstanceError::new.This checks if the error is an error response from the sled-agent
indicating the VMM is gone (as we did previously). In addition, if
the error is a communication error, it also checks if the sled has
been expunged by consulting the database. Eventually, I would also
kinda like to hang the "check if the sled is still in A0" code from
[nexus] fail instances if the sled they're on isn't...on #10334 off of this type instead of making that something that only
the
instance_watcherbackground task knows how to do, but I'drather do that in a follow-up change.
This is kinda why we don't just use the
progenitor-extrasGoneCheckstuff here, because we also want to keep the currenthandling of the
NO_SUCH_INSTANCEerror and the associated logic forconstructing
VmmFailureReasons from it.Updated all the call sites that use
SledAgentInstanceErrorto usethe new thing, which is why this diff is so huge. Sorry.
Finally, once all that was done, added a new
undo_instance_ensure_registeredhelper insagas::instance_commonfor actually doing the ensure-unregistered undo action. This has a a
retry loop that actually does the correct thing (as I described
above) instead of the kind of fractally-wrong thing that the
instance-startsaga's version of this used to do. Now, both sagasjust call this, and it does the right thing.
Return
Ok(())immediately and continue unwinding.Footnotes
Incidentally, this is one of the only pieces of this whole mess
that is not entirely my fault: although the code in question was
last touched by yours truly, there is a comment above it
explaining the rationale for the incorrect behavior. Although
this comment is also both incorrect in describing the code's
behavior and what it asserts that the code does would be
incorrect if we did do that, it is exceptionally well written in
a way where long-time fans of the show will immediately realize
we have just stumbled across a @gjcolumbo cameo appearance (hi
Greg!). ↩
With the exception of the
instance-watcherbackground task...butit only checks if the sled has been expunged before trying to
talk to the sled-agent, and doesn't check again upon encountering
a communication error. Sigh. ↩